feat: validate APISIX resources in webhooks#393
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ADC validation: new Client.Validate and executor.Validate paths, APISIX-specific validate requests, structured ADC validation error types, controller helpers to prepare resources for validation, an adcAdmissionValidator wired into webhooks, and supporting unit and e2e tests that assert admission deny/fail-open behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant K8sClient as Kubernetes Client
participant Webhook as Admission Webhook
participant ADCValidator as adcAdmissionValidator
participant Controller as Controller Helper
participant ADCClient as ADC Client
participant Executor as HTTP Executor
participant ADCServer as ADC / APISIX Server
K8sClient->>Webhook: submit resource (Route/Consumer/TLS)
Webhook->>ADCValidator: Validate(resource)
ADCValidator->>Controller: Prepare*ForValidation(resource)
Controller-->>ADCValidator: TranslateContext
ADCValidator->>ADCClient: build Task / configs
ADCClient->>ADCClient: write temp sync file, record IO metrics
ADCClient->>Executor: Validate(config, args)
Executor->>ADCServer: POST /validate or POST /apisix/admin/configs/validate
ADCServer-->>Executor: 2xx/400 + payload
Executor->>ADCClient: structured validation result or infra error
ADCClient-->>ADCValidator: aggregated ADCValidationErrors or error
ADCValidator-->>Webhook: error (deny) or nil (allow)
Webhook-->>K8sClient: admission response (allowed/denied)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-05-06T06:08:57Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
result: partial
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 0
Passed: 32
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests partially succeeded with 1 test skips. Extended tests partially
succeeded with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-05-06T06:09:51Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips.
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded. |
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-05-06T01:29:33Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- GatewayModifyListeners
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures.
- core:
failedTests:
- GRPCExactMethodMatching
- GRPCRouteHeaderMatching
- GRPCRouteListenerHostnameMatching
- GatewayModifyListeners
result: failure
statistics:
Failed: 4
Passed: 8
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 4 test failures. |
There was a problem hiding this comment.
Pull request overview
This PR adds ADC-backed admission validation to APISIX-related validating webhooks so that explicit validation failures are rejected while infrastructure/backend issues remain fail-open, and extends unit/e2e test coverage for these flows.
Changes:
- Introduce a shared
adcAdmissionValidatorto translate resources and call ADC validation during admission for ApisixRoute, ApisixConsumer, Consumer, and ApisixTls. - Extend ADC client/executor with a validation pathway (including APISIX standalone
/apisix/admin/configs/validatesupport) and richer validation error types. - Update and add unit + e2e tests to assert admission rejections for invalid resources and duplicate credentials.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/webhook/helpers.go | Adds a helper for asserting admission denials and non-existence of rejected resources. |
| test/e2e/webhook/consumer.go | Adds an e2e case for rejecting duplicate Consumer credentials. |
| test/e2e/webhook/apisixtls.go | Switches missing-secret behavior to rejection and adds invalid TLS-material rejection coverage. |
| test/e2e/webhook/apisixroute.go | Adds e2e denial coverage for ADC validation failures and adjusts missing-ref expectations. |
| test/e2e/webhook/apisixconsumer.go | Switches missing-secret behavior to rejection and adds duplicate-credential denial coverage. |
| internal/webhook/v1/consumer_webhook_test.go | Adds unit test coverage for duplicate key-auth credential denial. |
| internal/webhook/v1/consumer_webhook.go | Wires ADC validation into admission and adds duplicate key-auth enforcement. |
| internal/webhook/v1/apisixtls_webhook_test.go | Adds ADC-deny unit test and improves test ingress class setup. |
| internal/webhook/v1/apisixtls_webhook.go | Wires ADC validation into ApisixTls admission validation. |
| internal/webhook/v1/apisixroute_webhook_test.go | Adds ADC-deny + fail-open unit tests for ApisixRoute. |
| internal/webhook/v1/apisixroute_webhook.go | Wires ADC validation into ApisixRoute admission validation. |
| internal/webhook/v1/apisixconsumer_webhook_test.go | Adds ADC-deny unit test for ApisixConsumer and improves ingress class setup. |
| internal/webhook/v1/apisixconsumer_webhook.go | Wires ADC validation into ApisixConsumer admission validation. |
| internal/webhook/v1/adc_validation_test.go | Adds shared ADC mock helpers for webhook validation tests. |
| internal/webhook/v1/adc_validation.go | Introduces shared admission validator translating resources and calling ADC validate with fail-open semantics. |
| internal/types/error.go | Adds typed ADC validation error structures for propagating explicit validation failures. |
| internal/controller/webhook_validation.go | Adds shared “prepare-for-validation” helpers used by admission validation translation. |
| internal/adc/client/executor_test.go | Adds unit tests for APISIX standalone validate payload conversion for SSL certificates. |
| internal/adc/client/executor.go | Adds ADC validation execution (incl. APISIX validate endpoint) and validation response parsing. |
| internal/adc/client/client.go | Adds Client.Validate to drive executor validation per resolved config. |
Comments suppressed due to low confidence (1)
internal/adc/client/executor.go:524
buildHTTPRequestlogs the entirereqBody(includingTokenand full translatedConfig) at V(1). With admission validation this will run on user writes, so tokens/credentials can end up in logs. Please redact/remove sensitive fields from logs (token, credentials, TLS material), or replace with a minimal summary (cacheKey, backend, resource counts).
reqBody := ADCServerRequest{
Task: ADCServerTask{
Opts: ADCServerOpts{
Backend: config.BackendType,
Server: strings.Split(serverAddr, ","),
Token: config.Token,
LabelSelector: labels,
IncludeResourceType: types,
TlsSkipVerify: ptr.To(!tlsVerify),
CacheKey: config.Name,
},
Config: *resources,
},
}
e.log.V(1).Info("prepared request body", "body", reqBody)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "url", validateURL, | ||
| "server", serverAddr, | ||
| "cacheKey", config.Name, | ||
| "body", body, |
There was a problem hiding this comment.
buildAPISIXValidateRequest logs the full validate payload (including TLS certificates/private keys and other sensitive config) via e.log.V(1). This can leak secrets into controller logs during admission validation. Consider removing the payload from logs or logging only a redacted/summary view (e.g., counts/IDs) and never the key material.
| "body", body, | |
| "payloadBytes", len(jsonData), |
| } | ||
| body.SSLs = append(body.SSLs, sslMap) | ||
| } | ||
|
|
There was a problem hiding this comment.
buildAPISIXValidatePayload defines fields for global_rules and plugin_metadata in apisixValidateRequest, but never populates them from resources.GlobalRules / resources.PluginMetadata. This can make standalone validation incomplete or inconsistent with what would be synced. Populate those fields (and any other relevant top-level resources) so validation matches actual config behavior.
| for _, globalRule := range resources.GlobalRules { | |
| if globalRule == nil { | |
| continue | |
| } | |
| globalRuleMap, err := toMap(globalRule) | |
| if err != nil { | |
| return nil, err | |
| } | |
| body.GlobalRules = append(body.GlobalRules, globalRuleMap) | |
| } | |
| for _, pluginMetadata := range resources.PluginMetadata { | |
| if pluginMetadata == nil { | |
| continue | |
| } | |
| pluginMetadataMap, err := toMap(pluginMetadata) | |
| if err != nil { | |
| return nil, err | |
| } | |
| body.PluginMetadata = append(body.PluginMetadata, pluginMetadataMap) | |
| } |
| var consumers apisixv1alpha1.ConsumerList | ||
| if err := v.Client.List(ctx, &consumers); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for i := range consumers.Items { | ||
| existing := &consumers.Items[i] | ||
| if existing.Namespace == consumer.Namespace && existing.Name == consumer.Name { | ||
| continue | ||
| } | ||
| if !sameConsumerGatewayRef(existing, consumer) { | ||
| continue | ||
| } | ||
|
|
||
| existingKeys, err := v.extractKeyAuthKeys(ctx, existing) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
validateDuplicateKeyAuthCredentials lists all Consumer objects on every create/update, and then may fetch referenced Secrets for each existing Consumer. This is O(N) (and potentially N+1 Secret GETs) in the admission path and can become a latency/availability problem in clusters with many Consumers. Consider scoping the list when gatewayRef.namespace is empty (in-namespace only), and/or using an indexed field selector / cached index keyed by gatewayRef + key-auth key to avoid full scans and repeated secret fetches.
| Key string `json:"key"` | ||
| } | ||
| if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { | ||
| return "", nil |
There was a problem hiding this comment.
extractCredentialKey silently ignores JSON unmarshal errors by returning "" and nil. If a key-auth credential has malformed JSON, this bypasses duplicate-key detection and may allow invalid config through (especially when ADC validation fails open). Return the unmarshal error (or convert it into an admission denial with a clear message) so malformed credential configs are rejected deterministically.
| return "", nil | |
| return "", fmt.Errorf("failed to parse key-auth credential config for consumer %s/%s: %w", consumer.Namespace, consumer.Name, err) |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/webhook/v1/apisixconsumer_webhook_test.go (1)
45-66:⚠️ Potential issue | 🟡 MinorExtract
"apisix"to a constant to fix linter error.The string
"apisix"appears multiple times across test files. The linter flags this as agoconstviolation.🔧 Proposed fix
Add a constant at package level (e.g., in a shared test helper file or at the top of this file):
const defaultIngressClassName = "apisix"Then update the comparison and object creation:
for _, obj := range objects { ingressClass, ok := obj.(*networkingv1.IngressClass) - if ok && ingressClass.Name == "apisix" { + if ok && ingressClass.Name == defaultIngressClassName { hasManagedIngressClass = true break } } if !hasManagedIngressClass { managed = append(managed, &networkingv1.IngressClass{ ObjectMeta: metav1.ObjectMeta{ - Name: "apisix", + Name: defaultIngressClassName,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/webhook/v1/apisixconsumer_webhook_test.go` around lines 45 - 66, Introduce a package-level constant (e.g., const defaultIngressClassName = "apisix") and replace all literal "apisix" occurrences in this test with that constant: use defaultIngressClassName when comparing ingressClass.Name in the loop that sets hasManagedIngressClass and when constructing the &networkingv1.IngressClass{ObjectMeta: {Name: ..., Annotations: ...}, Spec: {Controller: config.ControllerConfig.ControllerName}} so the test no longer violates goconst; ensure the new constant is visible to other test files if they share the same package.
🧹 Nitpick comments (3)
internal/controller/webhook_validation.go (1)
44-49: Consider makingsupportsEndpointSliceconfigurable.The value
supportsEndpointSlice: trueis hardcoded here. If the cluster doesn't support EndpointSlices (older Kubernetes versions), this could cause issues during validation processing.Consider passing this as a parameter or deriving it from a shared configuration, similar to how the actual reconciler might determine this value at runtime. However, since EndpointSlices are GA since Kubernetes 1.21 and this is a recent codebase, hardcoding
trueis likely acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/webhook_validation.go` around lines 44 - 49, The hardcoded supportsEndpointSlice: true on the ApisixRouteReconciler instantiation in webhook_validation.go can break clusters without EndpointSlice support; change the construction of ApisixRouteReconciler to accept a configurable flag or derive it from shared cluster feature detection (e.g., read from a passed-in config struct or call a helper that checks API availability) and assign that value to ApisixRouteReconciler.supportsEndpointSlice instead of the literal true; update any callers that create this reconciler to pass the configured/derived boolean so validation honors cluster capabilities.internal/webhook/v1/consumer_webhook.go (1)
143-178: Consider performance impact of listing all Consumers on every validation.
validateDuplicateKeyAuthCredentialscallsv.Client.List(ctx, &consumers)without any field/label selector, fetching allConsumerresources cluster-wide. For clusters with many consumers, this could add significant latency to every create/update admission.Consider adding a label selector to filter by gateway reference, or implementing a cache/index if this becomes a bottleneck.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/webhook/v1/consumer_webhook.go` around lines 143 - 178, The code in validateDuplicateKeyAuthCredentials currently does a cluster-wide v.Client.List(ctx, &consumers) which is expensive; change the List invocation to only fetch relevant Consumers by constructing a selector (e.g., use client.MatchingLabels or client.MatchingFields) based on the consumer's gateway reference and namespace before calling v.Client.List, or switch to a cached/indexed lookup (via an informer/Lister) and query by the gatewayRef index; update the List call in validateDuplicateKeyAuthCredentials (and any helper that builds the query) so it only retrieves Consumers that could collide according to sameConsumerGatewayRef instead of listing all Consumers.internal/adc/client/executor.go (1)
328-340: Consider explicitly settingMinVersionon custom TLS client config.Go's default minimum TLS version is TLS 1.2, so the current code already meets the policy requirement. However, explicitly setting
MinVersion: tls.VersionTLS12improves code clarity and avoids reliance on runtime defaults that may change in future versions.Suggested fix
func (e *HTTPADCExecutor) newBackendHTTPClient(config adctypes.Config) *http.Client { transport := http.DefaultTransport.(*http.Transport).Clone() if !config.TlsVerify { if transport.TLSClientConfig == nil { - transport.TLSClientConfig = &tls.Config{} + transport.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + } + } else if transport.TLSClientConfig.MinVersion == 0 { + transport.TLSClientConfig.MinVersion = tls.VersionTLS12 } transport.TLSClientConfig.InsecureSkipVerify = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adc/client/executor.go` around lines 328 - 340, The TLS client config created in newBackendHTTPClient should explicitly set MinVersion to tls.VersionTLS12 instead of relying on runtime defaults; update the logic inside HTTPADCExecutor.newBackendHTTPClient where transport.TLSClientConfig is initialized so that if TLSClientConfig is nil you allocate it and set TLSClientConfig.MinVersion = tls.VersionTLS12 (and keep InsecureSkipVerify behavior unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adc/client/executor.go`:
- Around line 295-325: The log in buildAPISIXValidateRequest currently prints
sensitive data (config.Token and the request body containing creds/keys); change
it to log only non-sensitive metadata or a redacted payload: create a redacted
copy of the body (or call the type's MarshalLogObject/Redact method if
implemented) and replace config.Token with a placeholder (e.g., "<redacted>")
before logging, or remove body/token from the log entirely; update the logging
call in buildAPISIXValidateRequest (and the analogous logging in the other block
referred to) to use the redacted values so no raw tokens, consumer credentials,
certs, or private keys are emitted.
In `@internal/webhook/v1/adc_validation_test.go`:
- Around line 34-41: The helper withMockADCServer unnecessarily wraps the
already-typed http.HandlerFunc parameter when creating the test server; update
the call in withMockADCServer to pass handler directly to httptest.NewServer
(i.e., httptest.NewServer(handler)) and keep the rest of the function (t.Setenv,
t.Cleanup, return server.URL) unchanged to remove the redundant conversion.
In `@internal/webhook/v1/apisixconsumer_webhook.go`:
- Around line 47-63: The constructor stores initErr on
ApisixConsumerCustomValidator which causes a permanent hard-fail on any ADC init
error; remove the initErr field and stop storing that error in
NewApisixConsumerCustomValidator, and instead initialize adcValidator lazily
inside the validation entry points (e.g., the validator methods that run on
create/update) by calling newADCAdmissionValidator when adcValidator is nil; if
that init call returns an error, log the error with apisixConsumerLog and
proceed in "fail open" mode (do not block the request), ensuring reference to
the types/members ApisixConsumerCustomValidator,
NewApisixConsumerCustomValidator, adcValidator, and newADCAdmissionValidator so
you update the right places.
In `@internal/webhook/v1/apisixroute_webhook.go`:
- Around line 55-63: The validator currently stores initialization errors in
ApisixRouteCustomValidator.initErr (from newADCAdmissionValidator) and returns
them immediately in ValidateCreate/ValidateUpdate, causing permanent denials;
change ValidateCreate and ValidateUpdate on ApisixRouteCustomValidator to treat
initErr like runtime ADC unavailability: log the initErr via apisixRouteLog
(include context) and return nil (fail-open) instead of returning the initErr,
so runtime adcAdmissionValidator.Validate behavior and init-time failure are
consistent; keep the newADCAdmissionValidator and adcValidator fields as-is but
ensure all early-return paths reference initErr and use logging+nil return
rather than propagating the error.
In `@internal/webhook/v1/apisixtls_webhook.go`:
- Around line 82-87: The code currently returns v.initErr after collecting
warnings which turns ADC init failures into hard admission denials; change this
to fail-open: when v.initErr is non-nil, do not return the error — log or record
it but return the collected warnings with nil error and skip ADC validation
(i.e., do not call v.adcValidator.Validate when v.initErr != nil). Apply the
same change in the other similar block (lines 106-111) so ADC init/backend
problems do not permanently block admission or poison the validator.
In `@internal/webhook/v1/consumer_webhook.go`:
- Around line 49-65: The struct field initErr in ConsumerCustomValidator (and
the NewConsumerCustomValidator constructor) causes permanent admission denial
when initialization fails; remove the persistent initErr and instead handle
initialization failures by logging the error and returning a validator instance
that will operate in fail-open mode (e.g., set adcValidator to nil and proceed).
Update NewConsumerCustomValidator to not store initErr, and update any methods
that currently check initErr (such as Validate/Handle methods that may reside on
ConsumerCustomValidator) to treat a nil adcValidator as "uninitialized but
allow" rather than denying admission; keep reference.NewChecker and consumerLog
initialization as-is and ensure errors are logged via consumerLog.
- Around line 223-229: The code currently swallows json.Unmarshal errors when
parsing credential.Config.Raw into the local cfg struct and returns an empty
cfg.Key; change this so malformed JSON is surfaced: when
json.Unmarshal(credential.Config.Raw, &cfg) returns an error, return that error
(or at minimum log it with a contextual message including credential.Config.Raw)
instead of returning "", nil; update the function signature and callers as
required so callers can handle/report the unmarshalling error and include
references to json.Unmarshal, credential.Config.Raw, and cfg.Key when making the
change.
In `@test/e2e/webhook/apisixtls.go`:
- Around line 132-142: After recreating serverSecret with s.NewKubeTlsSecret,
wait until the cluster cache reflects the new Secret before creating the
ApisixTls to avoid race failures; implement a short polling loop that calls
s.GetResource("Secret", serverSecret) (or an existing helper like
s.GetKubeSecret) with a timeout/retry backoff and only proceed to call
s.CreateResourceFromString(tlsYAML) when the Secret is returned successfully,
ensuring you still check and Expect no error from that Get before continuing.
---
Outside diff comments:
In `@internal/webhook/v1/apisixconsumer_webhook_test.go`:
- Around line 45-66: Introduce a package-level constant (e.g., const
defaultIngressClassName = "apisix") and replace all literal "apisix" occurrences
in this test with that constant: use defaultIngressClassName when comparing
ingressClass.Name in the loop that sets hasManagedIngressClass and when
constructing the &networkingv1.IngressClass{ObjectMeta: {Name: ..., Annotations:
...}, Spec: {Controller: config.ControllerConfig.ControllerName}} so the test no
longer violates goconst; ensure the new constant is visible to other test files
if they share the same package.
---
Nitpick comments:
In `@internal/adc/client/executor.go`:
- Around line 328-340: The TLS client config created in newBackendHTTPClient
should explicitly set MinVersion to tls.VersionTLS12 instead of relying on
runtime defaults; update the logic inside HTTPADCExecutor.newBackendHTTPClient
where transport.TLSClientConfig is initialized so that if TLSClientConfig is nil
you allocate it and set TLSClientConfig.MinVersion = tls.VersionTLS12 (and keep
InsecureSkipVerify behavior unchanged).
In `@internal/controller/webhook_validation.go`:
- Around line 44-49: The hardcoded supportsEndpointSlice: true on the
ApisixRouteReconciler instantiation in webhook_validation.go can break clusters
without EndpointSlice support; change the construction of ApisixRouteReconciler
to accept a configurable flag or derive it from shared cluster feature detection
(e.g., read from a passed-in config struct or call a helper that checks API
availability) and assign that value to
ApisixRouteReconciler.supportsEndpointSlice instead of the literal true; update
any callers that create this reconciler to pass the configured/derived boolean
so validation honors cluster capabilities.
In `@internal/webhook/v1/consumer_webhook.go`:
- Around line 143-178: The code in validateDuplicateKeyAuthCredentials currently
does a cluster-wide v.Client.List(ctx, &consumers) which is expensive; change
the List invocation to only fetch relevant Consumers by constructing a selector
(e.g., use client.MatchingLabels or client.MatchingFields) based on the
consumer's gateway reference and namespace before calling v.Client.List, or
switch to a cached/indexed lookup (via an informer/Lister) and query by the
gatewayRef index; update the List call in validateDuplicateKeyAuthCredentials
(and any helper that builds the query) so it only retrieves Consumers that could
collide according to sameConsumerGatewayRef instead of listing all Consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cf73cf2-2f73-47f0-8938-83053a0638bc
📒 Files selected for processing (20)
internal/adc/client/client.gointernal/adc/client/executor.gointernal/adc/client/executor_test.gointernal/controller/webhook_validation.gointernal/types/error.gointernal/webhook/v1/adc_validation.gointernal/webhook/v1/adc_validation_test.gointernal/webhook/v1/apisixconsumer_webhook.gointernal/webhook/v1/apisixconsumer_webhook_test.gointernal/webhook/v1/apisixroute_webhook.gointernal/webhook/v1/apisixroute_webhook_test.gointernal/webhook/v1/apisixtls_webhook.gointernal/webhook/v1/apisixtls_webhook_test.gointernal/webhook/v1/consumer_webhook.gointernal/webhook/v1/consumer_webhook_test.gotest/e2e/webhook/apisixconsumer.gotest/e2e/webhook/apisixroute.gotest/e2e/webhook/apisixtls.gotest/e2e/webhook/consumer.gotest/e2e/webhook/helpers.go
| type ApisixConsumerCustomValidator struct { | ||
| Client client.Client | ||
| checker reference.Checker | ||
| Client client.Client | ||
| checker reference.Checker | ||
| adcValidator *adcAdmissionValidator | ||
| initErr error | ||
| } | ||
|
|
||
| var _ webhook.CustomValidator = &ApisixConsumerCustomValidator{} | ||
|
|
||
| func NewApisixConsumerCustomValidator(c client.Client) *ApisixConsumerCustomValidator { | ||
| adcValidator, err := newADCAdmissionValidator(c, apisixConsumerLog) | ||
| return &ApisixConsumerCustomValidator{ | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixConsumerLog), | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixConsumerLog), | ||
| adcValidator: adcValidator, | ||
| initErr: err, | ||
| } |
There was a problem hiding this comment.
Same initErr concern applies here.
See comment on apisixroute_webhook.go – storing and returning initErr on every validation call means ADC validator initialization failures will permanently block all ApisixConsumer writes rather than failing open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/apisixconsumer_webhook.go` around lines 47 - 63, The
constructor stores initErr on ApisixConsumerCustomValidator which causes a
permanent hard-fail on any ADC init error; remove the initErr field and stop
storing that error in NewApisixConsumerCustomValidator, and instead initialize
adcValidator lazily inside the validation entry points (e.g., the validator
methods that run on create/update) by calling newADCAdmissionValidator when
adcValidator is nil; if that init call returns an error, log the error with
apisixConsumerLog and proceed in "fail open" mode (do not block the request),
ensuring reference to the types/members ApisixConsumerCustomValidator,
NewApisixConsumerCustomValidator, adcValidator, and newADCAdmissionValidator so
you update the right places.
| func NewApisixRouteCustomValidator(c client.Client) *ApisixRouteCustomValidator { | ||
| adcValidator, err := newADCAdmissionValidator(c, apisixRouteLog) | ||
| return &ApisixRouteCustomValidator{ | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixRouteLog), | ||
| Client: c, | ||
| checker: reference.NewChecker(c, apisixRouteLog), | ||
| adcValidator: adcValidator, | ||
| initErr: err, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the best practice for Kubernetes admission webhook initialization errors - fail open or fail closed?
💡 Result:
The best practice for Kubernetes admission webhook initialization errors (timeouts, connection failures, or server errors) is to fail open for mutating webhooks (failurePolicy: Ignore) and fail closed for validating webhooks (failurePolicy: Fail, the default). For mutating webhooks, set failurePolicy to Ignore ("fail open") to prevent downtime from blocking compliant resource creation. Pair this with a separate validating webhook or controller to enforce policies on the final object state. This avoids disruptions from webhook unavailability while maintaining enforcement. For validating webhooks, use the default Fail ("fail closed") to ensure policy violations are blocked even during webhook issues, prioritizing security over availability. This distinction balances availability and security: mutations are best-effort and recoverable via validation, while validations are authoritative.
Citations:
- 1: https://kubernetes.io/docs/concepts/cluster-administration/admission-webhooks-good-practices/
- 2: https://k8s.ritovision.com/docs/concepts/cluster-administration/admission-webhooks-good-practices/
- 3: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/
- 4: https://kubernetes.io/docs/reference/kubernetes-api/extend-resources/mutating-webhook-configuration-v1
🏁 Script executed:
# Determine webhook type and find related configuration
fd -t f "apisixroute_webhook" --exec cat {} \;Repository: api7/api7-ingress-controller
Length of output: 14040
🏁 Script executed:
# Check consumer_webhook.go for comparison of initialization pattern
fd -t f "consumer_webhook" --exec head -100 {} \;Repository: api7/api7-ingress-controller
Length of output: 14771
🏁 Script executed:
# Search for any WebhookConfiguration or failurePolicy references related to ApisixRoute
rg -A 5 -B 5 "ApisixRoute.*Webhook" --type goRepository: api7/api7-ingress-controller
Length of output: 2154
🏁 Script executed:
# Check ADC validator implementation to understand error handling semantics
fd -t f "*adc*" --type go | head -5Repository: api7/api7-ingress-controller
Length of output: 242
🏁 Script executed:
# Find ADC validator implementation
fd -i "*adc*" | grep -v test | grep -v __pycache__Repository: api7/api7-ingress-controller
Length of output: 357
🏁 Script executed:
# Search for newADCAdmissionValidator and its error conditions
rg -B 5 -A 20 "func newADCAdmissionValidator"Repository: api7/api7-ingress-controller
Length of output: 1941
🏁 Script executed:
# Check if there are any comments or documentation about fail-open strategy
rg -i "fail.open|fail.closed|fail.close" --type goRepository: api7/api7-ingress-controller
Length of output: 194
🏁 Script executed:
# Look for any tests that verify initErr behavior specifically
rg "initErr" --type go -B 3 -A 3Repository: api7/api7-ingress-controller
Length of output: 7835
🏁 Script executed:
# Search for any comments or documentation about initErr strategy
rg "initialization|init.*error|fail.*open" internal/webhook -i -B 2 -A 2Repository: api7/api7-ingress-controller
Length of output: 1732
🏁 Script executed:
# Check ADC client creation to understand failure modes
cat internal/webhook/v1/adc_validation.go | head -60Repository: api7/api7-ingress-controller
Length of output: 2567
🏁 Script executed:
# Verify the actual webhook configuration - check for any failurePolicy clarity
rg "kubebuilder:webhook" internal/webhook/v1/ -A 1Repository: api7/api7-ingress-controller
Length of output: 4551
🏁 Script executed:
# Check the Validate method in adcAdmissionValidator to see runtime error handling
rg -A 30 "func.*adcAdmissionValidator.*Validate" internal/webhook/v1/adc_validation.go | head -50Repository: api7/api7-ingress-controller
Length of output: 840
Initialization errors create inconsistent fail behavior compared to runtime ADC unavailability.
The adcAdmissionValidator.Validate() method logs ADC connection errors (500) and returns nil, allowing admission (fail-open). However, newADCAdmissionValidator() initialization errors are stored in initErr and returned immediately on every ValidateCreate/ValidateUpdate call, permanently denying all writes.
This creates two failure modes for the same ADC integration:
- ADC unavailable at validation time → admission succeeds ✓
- ADC client initialization fails → admission fails ✗
Either both should fail-open (return nil to allow admission during transient failures), or initialization errors should be distinguished as configuration errors in code and documentation. Currently, the behavior is inconsistent with the fail-open philosophy applied to runtime errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/apisixroute_webhook.go` around lines 55 - 63, The
validator currently stores initialization errors in
ApisixRouteCustomValidator.initErr (from newADCAdmissionValidator) and returns
them immediately in ValidateCreate/ValidateUpdate, causing permanent denials;
change ValidateCreate and ValidateUpdate on ApisixRouteCustomValidator to treat
initErr like runtime ADC unavailability: log the initErr via apisixRouteLog
(include context) and return nil (fail-open) instead of returning the initErr,
so runtime adcAdmissionValidator.Validate behavior and init-time failure are
consistent; keep the newADCAdmissionValidator and adcValidator fields as-is but
ensure all early-return paths reference initErr and use logging+nil return
rather than propagating the error.
| type ConsumerCustomValidator struct { | ||
| Client client.Client | ||
| checker reference.Checker | ||
| Client client.Client | ||
| checker reference.Checker | ||
| adcValidator *adcAdmissionValidator | ||
| initErr error | ||
| } | ||
|
|
||
| var _ webhook.CustomValidator = &ConsumerCustomValidator{} | ||
|
|
||
| func NewConsumerCustomValidator(c client.Client) *ConsumerCustomValidator { | ||
| adcValidator, err := newADCAdmissionValidator(c, consumerLog) | ||
| return &ConsumerCustomValidator{ | ||
| Client: c, | ||
| checker: reference.NewChecker(c, consumerLog), | ||
| Client: c, | ||
| checker: reference.NewChecker(c, consumerLog), | ||
| adcValidator: adcValidator, | ||
| initErr: err, | ||
| } |
There was a problem hiding this comment.
Same initErr concern applies here.
See comment on apisixroute_webhook.go – storing and returning initErr causes permanent admission denial rather than fail-open behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/consumer_webhook.go` around lines 49 - 65, The struct
field initErr in ConsumerCustomValidator (and the NewConsumerCustomValidator
constructor) causes permanent admission denial when initialization fails; remove
the persistent initErr and instead handle initialization failures by logging the
error and returning a validator instance that will operate in fail-open mode
(e.g., set adcValidator to nil and proceed). Update NewConsumerCustomValidator
to not store initErr, and update any methods that currently check initErr (such
as Validate/Handle methods that may reside on ConsumerCustomValidator) to treat
a nil adcValidator as "uninitialized but allow" rather than denying admission;
keep reference.NewChecker and consumerLog initialization as-is and ensure errors
are logged via consumerLog.
| var cfg struct { | ||
| Key string `json:"key"` | ||
| } | ||
| if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { | ||
| return "", nil | ||
| } | ||
| return cfg.Key, nil |
There was a problem hiding this comment.
JSON unmarshal errors are silently ignored, potentially hiding malformed config.
If credential.Config.Raw contains invalid JSON, the error is swallowed and an empty key is returned. This could mask configuration errors from users.
Consider logging a warning or returning the error to surface malformed credential configs during validation.
🔧 Proposed fix to log malformed config
var cfg struct {
Key string `json:"key"`
}
if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil {
+ consumerLog.V(1).Info("failed to unmarshal key-auth config", "error", err, "consumer", consumer.Name)
return "", nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/webhook/v1/consumer_webhook.go` around lines 223 - 229, The code
currently swallows json.Unmarshal errors when parsing credential.Config.Raw into
the local cfg struct and returns an empty cfg.Key; change this so malformed JSON
is surfaced: when json.Unmarshal(credential.Config.Raw, &cfg) returns an error,
return that error (or at minimum log it with a contextual message including
credential.Config.Raw) instead of returning "", nil; update the function
signature and callers as required so callers can handle/report the unmarshalling
error and include references to json.Unmarshal, credential.Config.Raw, and
cfg.Key when making the change.
| By("replacing the secret with valid certificate material") | ||
| err = s.DeleteResource("Secret", serverSecret) | ||
| Expect(err).NotTo(HaveOccurred(), "deleting invalid server TLS secret") | ||
|
|
||
| serverCert, serverKey := s.GenerateCert(GinkgoT(), []string{host}) | ||
| err = s.NewKubeTlsSecret(serverSecret, serverCert.String(), serverKey.String()) | ||
| Expect(err).NotTo(HaveOccurred(), "creating valid server TLS secret") | ||
|
|
||
| By("creating corrected ApisixTls") | ||
| err = s.CreateResourceFromString(tlsYAML) | ||
| Expect(err).NotTo(HaveOccurred(), "creating corrected ApisixTls") |
There was a problem hiding this comment.
Wait for the recreated Secret before retrying admission.
This path recreates serverSecret and immediately submits ApisixTls. The webhook resolves Secrets through a cached client, so this can race cache propagation and make the e2e intermittently fail on stale secret state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/webhook/apisixtls.go` around lines 132 - 142, After recreating
serverSecret with s.NewKubeTlsSecret, wait until the cluster cache reflects the
new Secret before creating the ApisixTls to avoid race failures; implement a
short polling loop that calls s.GetResource("Secret", serverSecret) (or an
existing helper like s.GetKubeSecret) with a timeout/retry backoff and only
proceed to call s.CreateResourceFromString(tlsYAML) when the Secret is returned
successfully, ensuring you still check and Expect no error from that Get before
continuing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add ProviderTypeAPISIXStandalone skip guards to all ADC validation e2e tests so they are skipped on non-standalone backends that do not expose the /apisix/admin/configs/validate endpoint - Replace the 'duplicate credentials' consumer tests with invalid jwt-auth algorithm tests; APISIX validates plugin config schema in isolation and cannot detect cross-consumer key uniqueness, but it does reject unknown enum values (e.g. algorithm: INVALID_ALGO) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adc/client/executor.go`:
- Around line 328-341: The TLS config created in
HTTPADCExecutor.newBackendHTTPClient currently sets InsecureSkipVerify but does
not enforce a minimum TLS version; update newBackendHTTPClient to ensure the
TLSClientConfig has MinVersion = tls.VersionTLS12 (or higher) when creating or
augmenting transport.TLSClientConfig, and if transport.TLSClientConfig already
exists only set MinVersion when it is 0 to avoid overwriting explicit settings;
reference newBackendHTTPClient, HTTPADCExecutor, transport.TLSClientConfig and
tls.Config.MinVersion/tls.VersionTLS12.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51318ffa-868c-4719-99ba-f7e37b438dbe
📒 Files selected for processing (4)
internal/adc/client/executor.gointernal/adc/client/executor_test.gointernal/webhook/v1/adc_validation_test.gointernal/webhook/v1/apisixconsumer_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/webhook/v1/adc_validation_test.go
- internal/adc/client/executor_test.go
| func (e *HTTPADCExecutor) newBackendHTTPClient(config adctypes.Config) *http.Client { | ||
| transport := http.DefaultTransport.(*http.Transport).Clone() | ||
| if !config.TlsVerify { | ||
| if transport.TLSClientConfig == nil { | ||
| transport.TLSClientConfig = &tls.Config{} | ||
| } | ||
| transport.TLSClientConfig.InsecureSkipVerify = true | ||
| } | ||
|
|
||
| return &http.Client{ | ||
| Timeout: e.httpClient.Timeout, | ||
| Transport: transport, | ||
| } | ||
| } |
There was a problem hiding this comment.
Set minimum TLS version to 1.2 or higher.
The TLS configuration lacks a MinVersion, which defaults to TLS 1.0 when acting as a client. This is a weak cryptographic configuration per CWE-327.
🔒 Proposed fix
func (e *HTTPADCExecutor) newBackendHTTPClient(config adctypes.Config) *http.Client {
transport := http.DefaultTransport.(*http.Transport).Clone()
if !config.TlsVerify {
if transport.TLSClientConfig == nil {
- transport.TLSClientConfig = &tls.Config{}
+ transport.TLSClientConfig = &tls.Config{
+ MinVersion: tls.VersionTLS12,
+ }
}
transport.TLSClientConfig.InsecureSkipVerify = true
+ } else {
+ if transport.TLSClientConfig == nil {
+ transport.TLSClientConfig = &tls.Config{}
+ }
+ transport.TLSClientConfig.MinVersion = tls.VersionTLS12
}
return &http.Client{As per coding guidelines, "TLS and cryptographic configuration must use correct semantics... default TLS min version to 1.2 or higher".
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 331-331: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adc/client/executor.go` around lines 328 - 341, The TLS config
created in HTTPADCExecutor.newBackendHTTPClient currently sets
InsecureSkipVerify but does not enforce a minimum TLS version; update
newBackendHTTPClient to ensure the TLSClientConfig has MinVersion =
tls.VersionTLS12 (or higher) when creating or augmenting
transport.TLSClientConfig, and if transport.TLSClientConfig already exists only
set MinVersion when it is 0 to avoid overwriting explicit settings; reference
newBackendHTTPClient, HTTPADCExecutor, transport.TLSClientConfig and
tls.Config.MinVersion/tls.VersionTLS12.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var consumers apisixv1alpha1.ConsumerList | ||
| if err := v.Client.List(ctx, &consumers); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
validateDuplicateKeyAuthCredentials lists all Consumer objects cluster-wide and then iterates them to detect duplicates. On large clusters this makes admission latency O(total Consumers) (plus potential secret lookups), which can become a bottleneck. Consider adding a field index (e.g., by resolved GatewayRef) and using MatchingFields/scoped listing to narrow candidates before scanning.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/e2e-test-k8s.yml (1)
70-70: Use an isolated kubeconfig file for self-hosted runner stability.Line 70 is correct, but exporting to the default kubeconfig path can cause state bleed across runs on
self-hosted. Prefer an explicitKUBECONFIGfile and persist it for downstream e2e steps.♻️ Suggested hardening
- name: Launch Kind Cluster env: KIND_NODE_IMAGE: kindest/node:v1.18.15 + KIND_NAME: apisix-ingress-cluster + KUBECONFIG: ${{ runner.temp }}/kind-${{ github.run_id }}.kubeconfig run: | make kind-up - kind export kubeconfig --name apisix-ingress-cluster + kind export kubeconfig --name "${KIND_NAME}" --kubeconfig "${KUBECONFIG}" + echo "KUBECONFIG=${KUBECONFIG}" >> "${GITHUB_ENV}" kubectl wait --for=condition=Ready nodes --all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test-k8s.yml at line 70, The current use of "kind export kubeconfig --name apisix-ingress-cluster" writes to the default kubeconfig and can leak state on self-hosted runners; fix by directing kind to an explicit file and exporting that path for downstream steps: create a dedicated kube dir (e.g. .kube/config in the workspace), set the KUBECONFIG environment variable to that file before invoking the "kind export kubeconfig --name apisix-ingress-cluster" command, and persist that env/file for subsequent e2e steps (via workflow env or step outputs) so all later steps use the isolated kubeconfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/e2e-test-k8s.yml:
- Line 70: The current use of "kind export kubeconfig --name
apisix-ingress-cluster" writes to the default kubeconfig and can leak state on
self-hosted runners; fix by directing kind to an explicit file and exporting
that path for downstream steps: create a dedicated kube dir (e.g. .kube/config
in the workspace), set the KUBECONFIG environment variable to that file before
invoking the "kind export kubeconfig --name apisix-ingress-cluster" command, and
persist that env/file for subsequent e2e steps (via workflow env or step
outputs) so all later steps use the isolated kubeconfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad9e436c-5a93-4381-acc1-55002624b4af
📒 Files selected for processing (1)
.github/workflows/e2e-test-k8s.yml
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reconciler := &ApisixRouteReconciler{ | ||
| Client: c, | ||
| Log: log, | ||
| ICGV: networkingv1.SchemeGroupVersion, | ||
| supportsEndpointSlice: true, | ||
| } |
There was a problem hiding this comment.
PrepareApisixRouteForValidation hardcodes supportsEndpointSlice: true, which can break validation on older clusters (e.g., k8s 1.18 where discovery.k8s.io/v1 EndpointSlice isn’t served). This can cause admission to error/deny routes because endpoint discovery fails. Consider detecting EndpointSlice support (similar to the reconciler’s HasAPIResource check) or defaulting to false/fallback to Endpoints for webhook validation.
| func buildAPISIXValidatePayload(resources *adctypes.Resources) (*apisixValidateRequest, error) { | ||
| body := &apisixValidateRequest{} | ||
|
|
||
| for _, service := range resources.Services { |
There was a problem hiding this comment.
buildAPISIXValidatePayload populates routes/services/consumers/ssls, but it never includes resources.GlobalRules or resources.PluginMetadata even though the request struct has global_rules / plugin_metadata fields. This means standalone validation can silently skip validating those resources. Consider converting these fields into the payload as well (or remove the unused fields if unsupported).
| configs, err := v.buildIngressClassConfigs(ctx, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(configs) == 0 { | ||
| return nil, nil | ||
| } | ||
| tctx, err = controller.PrepareApisixRouteForValidation(ctx, v.kubeClient, v.log, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result, err = v.translator.TranslateApisixRoute(tctx, resource.DeepCopy()) |
There was a problem hiding this comment.
In buildTask for v2 resources, ingress class parameters/configs are processed twice: first via buildIngressClassConfigs(...), then again inside controller.Prepare...ForValidation(...) (which calls FindMatchingIngressClassByObject + ProcessIngressClassParameters). This doubles API calls during admission. Consider reusing the TranslateContext from Prepare...ForValidation to build configs (and drop buildIngressClassConfigs), or have Prepare...ForValidation return configs directly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configs, err := v.buildIngressClassConfigs(ctx, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(configs) == 0 { | ||
| return nil, nil | ||
| } | ||
| tctx, err = controller.PrepareApisixRouteForValidation(ctx, v.kubeClient, v.log, resource.DeepCopy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result, err = v.translator.TranslateApisixRoute(tctx, resource.DeepCopy()) | ||
| resourceTypes = append(resourceTypes, adctypes.TypeService) | ||
| if err != nil { |
There was a problem hiding this comment.
buildTask calls buildIngressClassConfigs() (which runs FindMatchingIngressClassByObject + ProcessIngressClassParameters) and then calls PrepareApisix*ForValidation() which repeats the same ingress class matching/parameter processing. This duplicates API reads and makes admission slower than necessary. Consider building configs from the TranslateContext returned by the Prepare...ForValidation helpers instead of re-processing the IngressClass twice.
| package main | ||
|
|
||
| import ( |
There was a problem hiding this comment.
This new Go file is missing the standard ASF license header that other cmd/ entrypoints in this repo include. Add the Apache Software Foundation header comment at the top for license compliance/consistency.
| @@ -44,16 +47,21 @@ func SetupConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore |
| @@ -44,16 +44,21 @@ func SetupApisixRouteWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1 |
| @@ -45,16 +45,21 @@ func SetupApisixTlsWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1 |
| @@ -45,16 +45,21 @@ func SetupApisixConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
There was a problem hiding this comment.
The kubebuilder webhook marker declares failurePolicy twice (failurePolicy=fail and failurePolicy=Ignore). This is ambiguous for generated manifests and can lead to an unexpected admission behavior. Keep only a single failurePolicy value (whichever is intended for this webhook).
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1 |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var errs types.ADCValidationErrors | ||
| for _, config := range task.Configs { | ||
| if config.BackendType == "" { | ||
| config.BackendType = c.defaultMode | ||
| } | ||
| if err := c.executor.Validate(ctx, config, args); err != nil { | ||
| var validationErr types.ADCValidationError | ||
| if errors.As(err, &validationErr) { | ||
| errs.Errors = append(errs.Errors, validationErr) | ||
| continue | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Client.Validate returns immediately on the first non-ADCValidationError from executor.Validate. This can cause a mixed outcome to fail-open: if one GatewayProxy config has an infra error (e.g., backend unavailable) but another config would return explicit validation errors, the infra error will short-circuit and the admission layer will treat it as an infra error and allow the request. Consider aggregating infra errors across configs and only returning them if no validation errors were found for any config (similar to how per-server validation is aggregated in runHTTPValidate).
|
|
||
| "github.com/gorilla/websocket" | ||
| ) | ||
|
|
||
| var upgrader = websocket.Upgrader{ | ||
| CheckOrigin: func(*http.Request) bool { | ||
| return true | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
websocket.Upgrader is configured with CheckOrigin always returning true, which disables the origin check entirely. Even for an e2e helper, this pattern is risky if the binary is ever reused outside tests; consider restricting origins (e.g., same-host) or making this explicitly configurable/documented as test-only behavior.
| "github.com/gorilla/websocket" | |
| ) | |
| var upgrader = websocket.Upgrader{ | |
| CheckOrigin: func(*http.Request) bool { | |
| return true | |
| }, | |
| } | |
| "net/url" | |
| "github.com/gorilla/websocket" | |
| ) | |
| func sameHostOrigin(r *http.Request) bool { | |
| origin := r.Header.Get("Origin") | |
| if origin == "" { | |
| return true | |
| } | |
| u, err := url.Parse(origin) | |
| if err != nil { | |
| return false | |
| } | |
| return u.Host == r.Host | |
| } | |
| var upgrader = websocket.Upgrader{ | |
| CheckOrigin: sameHostOrigin, | |
| } |
| func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error { | ||
| return WaitPodsAvailableWithTimeout(t, kubeOps, opts, time.Minute) |
There was a problem hiding this comment.
WaitPodsAvailable now hard-codes a 1 minute timeout, whereas the previous exponential backoff (500ms * 2^n, 8 steps) effectively waited ~2 minutes. This change can make e2e deployments flakier in slow CI clusters. Consider keeping the previous default (or increasing it) and using WaitPodsAvailableWithTimeout only at call sites that need custom bounds.
| func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error { | |
| return WaitPodsAvailableWithTimeout(t, kubeOps, opts, time.Minute) | |
| const waitPodsAvailableDefaultTimeout = 127500 * time.Millisecond | |
| func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error { | |
| return WaitPodsAvailableWithTimeout(t, kubeOps, opts, waitPodsAvailableDefaultTimeout) |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| .PHONY: build-e2e-echo-server-image | ||
| build-e2e-echo-server-image: | ||
| @CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/e2e-echo-server ./cmd/e2e-echo-server |
There was a problem hiding this comment.
build-e2e-echo-server-image hard-codes GOARCH=amd64. This will break running e2e tests on arm64 hosts/clusters (the scratch image will contain an amd64 binary). Consider using the existing $(GOARCH) Makefile variable (or building/loading a multi-arch image) so the echo-server image matches the target platform.
| @CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/e2e-echo-server ./cmd/e2e-echo-server | |
| @CGO_ENABLED=0 GOOS=linux GOARCH=$(GOARCH) go build -o bin/e2e-echo-server ./cmd/e2e-echo-server |
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), `duplicate key-auth credential key "shared-key"`) | ||
| require.Contains(t, err.Error(), "default/existing") | ||
| } |
There was a problem hiding this comment.
The new duplicate key-auth detection test only covers the inline credential.Config.Raw path. Since the implementation also supports extracting the key from credential.SecretRef (and has special handling for missing/invalid data), consider adding unit coverage for the SecretRef-based key extraction to ensure duplicate detection works for the common Secret-backed credential flow.
| } | |
| } | |
| func TestConsumerValidator_DenyDuplicateKeyAuthCredentialFromSecretRef(t *testing.T) { | |
| existing := &apisixv1alpha1.Consumer{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "existing", | |
| Namespace: "default", | |
| }, | |
| Spec: apisixv1alpha1.ConsumerSpec{ | |
| GatewayRef: apisixv1alpha1.GatewayRef{Name: "test-gateway"}, | |
| Credentials: []apisixv1alpha1.Credential{{ | |
| Type: "key-auth", | |
| SecretRef: &apisixv1alpha1.SecretReference{ | |
| Name: "existing-key-secret", | |
| }, | |
| }}, | |
| }, | |
| } | |
| consumer := &apisixv1alpha1.Consumer{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "demo", | |
| Namespace: "default", | |
| }, | |
| Spec: apisixv1alpha1.ConsumerSpec{ | |
| GatewayRef: apisixv1alpha1.GatewayRef{Name: "test-gateway"}, | |
| Credentials: []apisixv1alpha1.Credential{{ | |
| Type: "key-auth", | |
| SecretRef: &apisixv1alpha1.SecretReference{ | |
| Name: "demo-key-secret", | |
| }, | |
| }}, | |
| }, | |
| } | |
| validator := buildConsumerValidator( | |
| t, | |
| existing, | |
| &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "existing-key-secret", Namespace: "default"}, | |
| Data: map[string][]byte{ | |
| "key": []byte("shared-key"), | |
| }, | |
| }, | |
| &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "demo-key-secret", Namespace: "default"}, | |
| Data: map[string][]byte{ | |
| "key": []byte("shared-key"), | |
| }, | |
| }, | |
| ) | |
| warnings, err := validator.ValidateCreate(context.Background(), consumer) | |
| require.Empty(t, warnings) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), `duplicate key-auth credential key "shared-key"`) | |
| require.Contains(t, err.Error(), "default/existing") | |
| } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Revert CI/deployment changes that are not needed for the webhook ADC validation feature and work fine on master: - Revert docker login steps back to docker/login-action@v3 in all workflow files (apisix-conformance-test, apisix-e2e-test, conformance-test, e2e-test, e2e-test-k8s) - Revert e2e-test-k8s.yml kind cluster setup to original approach - Revert e2e-test-k8s.yml postgres image mirror preloading - Revert spell-checker.yml to use wget-based misspell install - Revert Makefile pull-infra-images to simple docker pull (keep jmalloc/echo-server removed since it is now built locally) - Revert Makefile ADC binary download to simple curl | tar Retained changes required for the feature: - build-e2e-echo-server-image target - kind-load-images dependency on build-e2e-echo-server-image Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Foo Bar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
The 'ApisixTls and Ingress with same certificate but different hosts' test was consistently failing due to two compounding issues: 1. Control-plane / data-plane propagation delay: the control plane confirms the api7.com SSL object, but APISIX data plane may not have loaded it yet when the HTTPS request is made. 2. Non-retryable Eventually block: NewAPISIXHttpsClient uses GinkgoT() as the httpexpect reporter. On TLS error, httpexpect calls GinkgoT().Fatalf() -> runtime.Goexit(), which exits the goroutine immediately. gomega's Eventually cannot retry because the goroutine is gone. Fix: replace the raw Eventually+httpexpect blocks with s.RequestAssert, which already exists in the scaffold and uses ErrorReporter (stores errors instead of calling FailNow). Transient TLS errors are now returned as retryable errors, letting Eventually poll until the data plane is fully ready. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When ApisixTls references secrets that do not exist yet, the webhook should warn (not deny). The ADC validator calls PrepareApisixTlsForValidation which in turn calls validateSecret, which returns NotFound and causes admission denial - breaking the original warn-on-missing-secret behavior. Fix: skip ADC validation when collectWarnings already detected missing secrets. The translator cannot load cert/key material in that case, so ADC validation would always fail anyway. The existing warnings are sufficient to inform the user. Also fix initErr fail-open: a validator initialization failure should allow admission (return warnings, nil) rather than hard-deny every write. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| warnings := v.collectWarnings(ctx, tls) | ||
| // Skip ADC validation when secrets are missing: the translator cannot | ||
| // load cert/key material, so validation would always fail. The missing- | ||
| // secret warnings are sufficient to inform the user. | ||
| if v.initErr != nil || len(warnings) > 0 { | ||
| return warnings, nil | ||
| } |
| @@ -45,16 +45,21 @@ func SetupApisixTlsWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixtls,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixtlses,verbs=create;update,versions=v2,name=vapisixtls-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| warnings := v.collectWarnings(ctx, route) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } |
| @@ -44,16 +44,21 @@ func SetupApisixRouteWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixroute,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixroutes,verbs=create;update,versions=v2,name=vapisixroute-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| warnings := v.collectWarnings(ctx, consumer) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } |
| @@ -45,16 +45,21 @@ func SetupApisixConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v2-apisixconsumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=apisixconsumers,verbs=create;update,versions=v2,name=vapisixconsumer-v2.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
| warnings := v.collectWarnings(ctx, consumer) | ||
| if v.initErr != nil { | ||
| return warnings, v.initErr | ||
| } |
| @@ -44,16 +47,21 @@ func SetupConsumerWebhookWithManager(mgr ctrl.Manager) error { | |||
| // +kubebuilder:webhook:path=/validate-apisix-apache-org-v1alpha1-consumer,mutating=false,failurePolicy=fail,sideEffects=None,groups=apisix.apache.org,resources=consumers,verbs=create;update,versions=v1alpha1,name=vconsumer-v1alpha1.kb.io,admissionReviewVersions=v1,failurePolicy=Ignore | |||
Summary
Testing
go test ./internal/adc/client ./internal/webhook/v1/...go test ./test/e2e/apisix -ginkgo.focus='Test (ApisixRoute|ApisixConsumer|Consumer|ApisixTls) Webhook'(currently 4 passed / 4 failed; route and consumer standalone validate payloads still need follow-up)Summary by CodeRabbit
New Features
Improvements
Tests